Skip to content

Is in ancestor caching #30

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Is in ancestor caching #30

wants to merge 6 commits into from

Conversation

ferrine
Copy link
Member

@ferrine ferrine commented Nov 24, 2022 •
edited
Loading

Implemented a refactored version of the util to make the complete split for the graph

Here are a few important guidelines and requirements to check before your PR can be merged:

  • There is an informative high-level description of the changes.
  • The description and/or commit message(s) references the relevant GitHub issue(s).
  • pre-commit is installed and set up.
  • The commit messages follow these guidelines.
  • The commits correspond to relevant logical changes, and there are no commits that fix changes introduced by other commits in the same branch/BR.
  • There are tests covering the changes introduced in the PR.

Don't worry, your PR doesn't need to be in perfect order to submit it. As development progresses and/or reviewers request changes, you can always rewrite the history of your feature/PR branches.

If your PR is an ongoing effort and you would like to involve us in the process, simply make it a draft PR.

@ricardoV94
Copy link
Member

Can we add it as a separate function?

@ferrine
Copy link
Member Author

ferrine commented Nov 24, 2022 •
edited
Loading

Can we add it as a separate function?

Hmm, this will duplicate the functionality, doesn't it?

@ricardoV94
Copy link
Member

ricardoV94 commented Nov 24, 2022 •
edited
Loading

Can we add it as a separate function?

Hmm, this will duplicate the functionality, doesn't it?

It makes reviewing easier, as we don't need to worry about back-compatibility.

Otherwise can you provide more context to motivate these changes? Why is this needed? There's no issue associated with it.

@ferrine
Copy link
Member Author

ferrine commented Nov 24, 2022 •
edited
Loading

Can we add it as a separate function?

Hmm, this will duplicate the functionality, doesn't it?

It makes reviewing easier, as we don't need to worry about back-compatibility.

Otherwise can you provide more context to motivate these changes? Why is this needed? There's no issue associated with it.

If you traverse the graph and query if an apply depends on other apply and do that frequently, known_(in)dependent cut the query costs

The change is backward compatible

@ricardoV94
Copy link
Member

Can you clarify where this was a bottleneck that warrants the extra complexity?

@ferrine
Copy link
Member Author

ferrine commented Nov 24, 2022

Can you clarify where this was a bottleneck that warrants the extra complexity?

Adding the fuctionality pytensor.graph.replace (#19) I am not sure I can write clean code avoiding multiple is_ancestor calls. However, I still believe there might be a solution to figure it out from toposort and extra containers.

@ferrine
Copy link
Member Author

ferrine commented Nov 24, 2022

Here is the candidate implementation for the #19 https://github.com/pymc-devs/pytensor/blob/graph-replace/pytensor/graph/basic.py#L1924, I use the updated version of the is_in_ancestor and it helps there, the thing especially needed is multiple inputs for the f_apply arg

@ferrine ferrine force-pushed the is-in-ancestor-caching branch from 8288f46 to 04f6d1b Compare November 29, 2022 13:19
@ferrine ferrine closed this Nov 29, 2022
@ferrine ferrine deleted the is-in-ancestor-caching branch November 30, 2022 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ferrine @ricardoV94